Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade winit to 0.30.2 #4849

Merged
merged 26 commits into from
Jul 31, 2024
Merged

Upgrade winit to 0.30.2 #4849

merged 26 commits into from
Jul 31, 2024

Conversation

ArthurBrussee
Copy link
Contributor

@ArthurBrussee ArthurBrussee commented Jul 18, 2024

Hiya,

I need new winit for a specific fix for a android_native_actvity. There are already two PRs, but both don't seem to have a lot of movement, or are entirely complete:

#4466
Seems to have gone stale & is missing some bits.

#4702
Also seems stale (if less so), and is missing a refactor to run_on_demand. I also think the accesskit integration has a mistake and can't be enabled. I've marked them as a co-author on this as I started from this branch. (I think! Haven't done that on git before...).

Sorry for the wall of text but just dumping some details / thoughts here:

  • There's an issue with creating child windows in winit 0.30.1 and up on macOS. The multiple_viewports, "create immediate viewport" example crashes on anything later 0.30.1, with a stack overflow in unsafe code. I've create a winit issue, it might already be fixed in 0.31.0 but I can't test as 0.31 will likely require another refactoring. For now I have just pinned things to 0.30.0 exatly.

  • Winit has deprecated run_on_demand, instead requiring the ApplicationHandler interface. In 0.31.0 run_on_demand is removed. I've refactored both the integration and the WinitApp trait to follow this pattern. I've left user_events a bit more opaque, as it seems 0.31.0 is doing a rework of UserEvents too.

  • I've used the new lazy init approach for access kit from this branch https://github.com/mwcampbell/egui/tree/accesskit-new-lazy-init and marked Matt as co-author, thanks Matt!

  • There was very similair but not quite the same code for run_and_return and run_and_exit. I've merged them, but looking at the github issues graveyard it seems vey finnicky. I hope this is more robust than before but it's a bit scary.

  • when receiving new_events this also used to check the redraw timing dictionary. That doesn't seem necesarry so left this out, but that is a slight behaviour change?

  • I have reeneabled serial_windows on macOS. I wondered whether it was fixed after this PR and does seem to be! However, even before this PR it seems to work, so maybe winit has sorted things out before that... Windows also works fine now without the extra hack.

  • I've done a very basic test of AccessKit on Windows and screen reader seems ok but I'm really not knowleadgable enough to say whether it's all good or not.

  • I've tested cargo tests & all examples on Windows & macOS, and ran a basic Android app. Still, testing native platforms is wel... hard so if anyone can test linux / iOs / older mac versions / windows 10 would probably be a good idea!

  • For consistencys sake I've made all event like functions in WinitApp return a Result<EventResult>. There's quite a bit of Ok-wrapping now, maybe too annoying? Not sure.

Thank you for having a look!

Tested on

TODO

  • Fix "follow system theme" not working on initial startup (winit issue, pinning to 0.30.2 for now).
  • Fix request_repaint_after

Update to latest `accesskit` and `accesskit_winit`
Tested on WSL wayland. Required an annoying workaround for nix/fs :/
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
examples/serial_windows/src/main.rs Show resolved Hide resolved
crates/egui-winit/Cargo.toml Outdated Show resolved Hide resolved
@ArthurBrussee ArthurBrussee changed the title Upgrade winit to 0.30.0 Upgrade winit to 0.30.4 Jul 26, 2024
@emilk emilk added eframe Relates to epi and eframe egui-winit porblems related to winit dependencies Pull requests that update a dependency file labels Jul 30, 2024
@emilk
Copy link
Owner

emilk commented Jul 30, 2024

I found a couple of issues during testing; added them under the TODO heading in the PR description. I'll do some more testing and code review soon™️. I'd like to get this in as soon as possible!

In the previous code, new_events did check repaint times, this was lost in the translation.

Seperate out this check from handle_event_result as we don't need the complexity of that whole shebang.
@torokati44
Copy link
Contributor

Oh that's just the result of GNOME folks insisting that window decorations have to be done by the clients (applications), and not by the server (the compositor / window manager), lest they may be too consistent. 🙄 Window decorations look normal on KDE.

See: rust-windowing/winit#1967

@emilk
Copy link
Owner

emilk commented Jul 31, 2024

Awesome - thanks for the help with the testing! Let's go 🚀

@emilk emilk merged commit 6f2f006 into emilk:master Jul 31, 2024
19 checks passed
@mwcampbell
Copy link
Contributor

I just did a quick test of accessibility with a screen reader on Windows, and it looks good. Thanks @ArthurBrussee for getting this across the finish line.

@emilk emilk mentioned this pull request Jul 31, 2024
1 task
@bash bash mentioned this pull request Aug 4, 2024
1 task
emilk pushed a commit that referenced this pull request Aug 5, 2024
Since #4849, running `./scripts/check.sh` fails with:
```
...

error[E0277]: the trait bound `ActiveEventLoop: raw_window_handle::borrowed::HasDisplayHandle` is not satisfied
  --> crates/egui_glow/src/winit.rs:43:13
   |
43 |             event_loop,
   |             ^^^^^^^^^^ the trait `raw_window_handle::borrowed::HasDisplayHandle` is not implemented for `ActiveEventLoop`
   |
   = help: the following other types implement trait `raw_window_handle::borrowed::HasDisplayHandle`:
             raw_window_handle::borrowed::DisplayHandle<'a>
             &H
             &mut H
   = note: required for the cast from `&ActiveEventLoop` to `&dyn raw_window_handle::borrowed::HasDisplayHandle`
```

This PR adds the missing `rwh_06` to the winit dependency (in
egui-glow).

* [x] I have followed the instructions in the PR template
@torokati44
Copy link
Contributor

Sounds good! I've pinned to 0.30.2 with a comment, will keep an eye on the 0.30.5.

Soon! ™️ @ArthurBrussee
rust-windowing/winit#3849

@ArthurBrussee
Copy link
Contributor Author

Oh nice! Though seems actually so far 0.30.5 doesn't have the fix this needs alas. Have asked there whether it still might be included.

@torokati44
Copy link
Contributor

'Tis out!

emilk pushed a commit that referenced this pull request Aug 9, 2024
This updates winit to 0.30.5. 

#4849 Had to pin the version to
0.30.2, as a Winit patch changed the behavior of selecting a theme.
Winit 0.30.5 reverts this, so we could stick with `window.theme()`, but
the newly added `ActiveEventLoop::system_theme` is more like what egui
wants anyway, as individual windows can have theme overrides.

Also bump `smithay-clipboard` to prevent some now duplicate
dependencies.
rib pushed a commit to EmbarkStudios/egui that referenced this pull request Sep 30, 2024
* Closes emilk#1918
* Closes emilk#4437
* Closes emilk#4709
* [x] I have followed the instructions in the PR template

Hiya,

I need new winit for a specific fix for a android_native_actvity. There
are already two PRs, but both don't seem to have a lot of movement, or
are entirely complete:

emilk#4466
Seems to have gone stale & is missing some bits.

emilk#4702
Also seems stale (if less so), and is missing a refactor to
run_on_demand. I also *think* the accesskit integration has a mistake
and can't be enabled. I've marked them as a co-author on this as I
started from this branch. (I think! Haven't done that on git before...).

Sorry for the wall of text but just dumping some details / thoughts
here:

- There's an issue with creating child windows in winit 0.30.1 and up on
macOS. The multiple_viewports, "create immediate viewport" example
crashes on anything later 0.30.1, with a stack overflow in unsafe code.
I've create [a winit
issue](rust-windowing/winit#3800), it *might*
already be fixed in 0.31.0 but I can't test as 0.31 will likely require
another refactoring. For now I have just pinned things to 0.30.0 exatly.

- Winit has deprecated run_on_demand, instead requiring the
ApplicationHandler interface. In 0.31.0 run_on_demand is removed. I've
refactored both the integration and the WinitApp trait to follow this
pattern. I've left user_events a bit more opaque, as it seems 0.31.0 is
doing a rework of UserEvents too.

- I've used the new lazy init approach for access kit from this branch
https://github.com/mwcampbell/egui/tree/accesskit-new-lazy-init and
marked Matt as co-author, thanks Matt!

- There was very similair but not quite the same code for run_and_return
and run_and_exit. I've merged them, but looking at the github issues
graveyard it seems vey finnicky. I *hope* this is more robust than
before but it's a bit scary.

- when receiving new_events this also used to check the redraw timing
dictionary. That doesn't seem necesarry so left this out, but that is a
slight behaviour change?

- I have reeneabled serial_windows on macOS. I wondered whether it was
fixed after this PR and does seem to be! However, even before this PR it
seems to work, so maybe winit has sorted things out before that...
Windows also works fine now without the extra hack.

- I've done a very basic test of AccessKit on Windows and screen reader
seems ok but I'm really not knowleadgable enough to say whether it's all
good or not.

- I've tested cargo tests & all examples on Windows & macOS, and ran a
basic Android app. Still, testing native platforms is wel... hard so if
anyone can test linux / iOs / older mac versions / windows 10 would
probably be a good idea!

- For consistencys sake I've made all event like functions in WinitApp
return a `Result<EventResult>`. There's quite a bit of Ok-wrapping now,
maybe too annoying? Not sure.

Thank you for having a look!

* [x] macOS
* [x] Windows
* [x] Wayland (thanks [SiebenCorgie](https://github.com/SiebenCorgie))
* [x] X11 (thanks
[crumblingstatue](https://github.com/crumblingstatue)!,
[SiebenCorgie](https://github.com/SiebenCorgie))

* [x] Fix "follow system theme" not working on initial startup (winit
issue, pinning to 0.30.2 for now).
* [x] Fix `request_repaint_after`

---------

Co-authored-by: mwcampbell <mattcampbell@pobox.com>
Co-authored-by: j-axa <josef.axa@gmail.com>
Co-authored-by: DataTriny <datatriny@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
486c pushed a commit to 486c/egui that referenced this pull request Oct 9, 2024
* Closes emilk#1918
* Closes emilk#4437
* Closes emilk#4709
* [x] I have followed the instructions in the PR template

Hiya,

I need new winit for a specific fix for a android_native_actvity. There
are already two PRs, but both don't seem to have a lot of movement, or
are entirely complete:

emilk#4466
Seems to have gone stale & is missing some bits.

emilk#4702
Also seems stale (if less so), and is missing a refactor to
run_on_demand. I also *think* the accesskit integration has a mistake
and can't be enabled. I've marked them as a co-author on this as I
started from this branch. (I think! Haven't done that on git before...).

Sorry for the wall of text but just dumping some details / thoughts
here:

- There's an issue with creating child windows in winit 0.30.1 and up on
macOS. The multiple_viewports, "create immediate viewport" example
crashes on anything later 0.30.1, with a stack overflow in unsafe code.
I've create [a winit
issue](rust-windowing/winit#3800), it *might*
already be fixed in 0.31.0 but I can't test as 0.31 will likely require
another refactoring. For now I have just pinned things to 0.30.0 exatly.

- Winit has deprecated run_on_demand, instead requiring the
ApplicationHandler interface. In 0.31.0 run_on_demand is removed. I've
refactored both the integration and the WinitApp trait to follow this
pattern. I've left user_events a bit more opaque, as it seems 0.31.0 is
doing a rework of UserEvents too.

- I've used the new lazy init approach for access kit from this branch
https://github.com/mwcampbell/egui/tree/accesskit-new-lazy-init and
marked Matt as co-author, thanks Matt!

- There was very similair but not quite the same code for run_and_return
and run_and_exit. I've merged them, but looking at the github issues
graveyard it seems vey finnicky. I *hope* this is more robust than
before but it's a bit scary.

- when receiving new_events this also used to check the redraw timing
dictionary. That doesn't seem necesarry so left this out, but that is a
slight behaviour change?

- I have reeneabled serial_windows on macOS. I wondered whether it was
fixed after this PR and does seem to be! However, even before this PR it
seems to work, so maybe winit has sorted things out before that...
Windows also works fine now without the extra hack.

- I've done a very basic test of AccessKit on Windows and screen reader
seems ok but I'm really not knowleadgable enough to say whether it's all
good or not.

- I've tested cargo tests & all examples on Windows & macOS, and ran a
basic Android app. Still, testing native platforms is wel... hard so if
anyone can test linux / iOs / older mac versions / windows 10 would
probably be a good idea!

- For consistencys sake I've made all event like functions in WinitApp
return a `Result<EventResult>`. There's quite a bit of Ok-wrapping now,
maybe too annoying? Not sure.

Thank you for having a look!

* [x] macOS
* [x] Windows
* [x] Wayland (thanks [SiebenCorgie](https://github.com/SiebenCorgie))
* [x] X11 (thanks
[crumblingstatue](https://github.com/crumblingstatue)!,
[SiebenCorgie](https://github.com/SiebenCorgie))

* [x] Fix "follow system theme" not working on initial startup (winit
issue, pinning to 0.30.2 for now).
* [x] Fix `request_repaint_after`

---------

Co-authored-by: mwcampbell <mattcampbell@pobox.com>
Co-authored-by: j-axa <josef.axa@gmail.com>
Co-authored-by: DataTriny <datatriny@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
486c pushed a commit to 486c/egui that referenced this pull request Oct 9, 2024
Since emilk#4849, running `./scripts/check.sh` fails with:
```
...

error[E0277]: the trait bound `ActiveEventLoop: raw_window_handle::borrowed::HasDisplayHandle` is not satisfied
  --> crates/egui_glow/src/winit.rs:43:13
   |
43 |             event_loop,
   |             ^^^^^^^^^^ the trait `raw_window_handle::borrowed::HasDisplayHandle` is not implemented for `ActiveEventLoop`
   |
   = help: the following other types implement trait `raw_window_handle::borrowed::HasDisplayHandle`:
             raw_window_handle::borrowed::DisplayHandle<'a>
             &H
             &mut H
   = note: required for the cast from `&ActiveEventLoop` to `&dyn raw_window_handle::borrowed::HasDisplayHandle`
```

This PR adds the missing `rwh_06` to the winit dependency (in
egui-glow).

* [x] I have followed the instructions in the PR template
486c pushed a commit to 486c/egui that referenced this pull request Oct 9, 2024
This updates winit to 0.30.5. 

emilk#4849 Had to pin the version to
0.30.2, as a Winit patch changed the behavior of selecting a theme.
Winit 0.30.5 reverts this, so we could stick with `window.theme()`, but
the newly added `ActiveEventLoop::system_theme` is more like what egui
wants anyway, as individual windows can have theme overrides.

Also bump `smithay-clipboard` to prevent some now duplicate
dependencies.
hacknus pushed a commit to hacknus/egui that referenced this pull request Oct 30, 2024
* Closes emilk#1918
* Closes emilk#4437
* Closes emilk#4709
* [x] I have followed the instructions in the PR template

Hiya,

I need new winit for a specific fix for a android_native_actvity. There
are already two PRs, but both don't seem to have a lot of movement, or
are entirely complete:

emilk#4466
Seems to have gone stale & is missing some bits.

emilk#4702
Also seems stale (if less so), and is missing a refactor to
run_on_demand. I also *think* the accesskit integration has a mistake
and can't be enabled. I've marked them as a co-author on this as I
started from this branch. (I think! Haven't done that on git before...).

Sorry for the wall of text but just dumping some details / thoughts
here:

- There's an issue with creating child windows in winit 0.30.1 and up on
macOS. The multiple_viewports, "create immediate viewport" example
crashes on anything later 0.30.1, with a stack overflow in unsafe code.
I've create [a winit
issue](rust-windowing/winit#3800), it *might*
already be fixed in 0.31.0 but I can't test as 0.31 will likely require
another refactoring. For now I have just pinned things to 0.30.0 exatly.

- Winit has deprecated run_on_demand, instead requiring the
ApplicationHandler interface. In 0.31.0 run_on_demand is removed. I've
refactored both the integration and the WinitApp trait to follow this
pattern. I've left user_events a bit more opaque, as it seems 0.31.0 is
doing a rework of UserEvents too.

- I've used the new lazy init approach for access kit from this branch
https://github.com/mwcampbell/egui/tree/accesskit-new-lazy-init and
marked Matt as co-author, thanks Matt!

- There was very similair but not quite the same code for run_and_return
and run_and_exit. I've merged them, but looking at the github issues
graveyard it seems vey finnicky. I *hope* this is more robust than
before but it's a bit scary.

- when receiving new_events this also used to check the redraw timing
dictionary. That doesn't seem necesarry so left this out, but that is a
slight behaviour change?

- I have reeneabled serial_windows on macOS. I wondered whether it was
fixed after this PR and does seem to be! However, even before this PR it
seems to work, so maybe winit has sorted things out before that...
Windows also works fine now without the extra hack.

- I've done a very basic test of AccessKit on Windows and screen reader
seems ok but I'm really not knowleadgable enough to say whether it's all
good or not.

- I've tested cargo tests & all examples on Windows & macOS, and ran a
basic Android app. Still, testing native platforms is wel... hard so if
anyone can test linux / iOs / older mac versions / windows 10 would
probably be a good idea!

- For consistencys sake I've made all event like functions in WinitApp
return a `Result<EventResult>`. There's quite a bit of Ok-wrapping now,
maybe too annoying? Not sure.

Thank you for having a look!

# Tested on
* [x] macOS
* [x] Windows
* [x] Wayland (thanks [SiebenCorgie](https://github.com/SiebenCorgie))
* [x] X11 (thanks
[crumblingstatue](https://github.com/crumblingstatue)!,
[SiebenCorgie](https://github.com/SiebenCorgie))


# TODO
* [x] Fix "follow system theme" not working on initial startup (winit
issue, pinning to 0.30.2 for now).
* [x] Fix `request_repaint_after`

---------

Co-authored-by: mwcampbell <mattcampbell@pobox.com>
Co-authored-by: j-axa <josef.axa@gmail.com>
Co-authored-by: DataTriny <datatriny@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
hacknus pushed a commit to hacknus/egui that referenced this pull request Oct 30, 2024
Since emilk#4849, running `./scripts/check.sh` fails with:
```
...

error[E0277]: the trait bound `ActiveEventLoop: raw_window_handle::borrowed::HasDisplayHandle` is not satisfied
  --> crates/egui_glow/src/winit.rs:43:13
   |
43 |             event_loop,
   |             ^^^^^^^^^^ the trait `raw_window_handle::borrowed::HasDisplayHandle` is not implemented for `ActiveEventLoop`
   |
   = help: the following other types implement trait `raw_window_handle::borrowed::HasDisplayHandle`:
             raw_window_handle::borrowed::DisplayHandle<'a>
             &H
             &mut H
   = note: required for the cast from `&ActiveEventLoop` to `&dyn raw_window_handle::borrowed::HasDisplayHandle`
```

This PR adds the missing `rwh_06` to the winit dependency (in
egui-glow).

* [x] I have followed the instructions in the PR template
hacknus pushed a commit to hacknus/egui that referenced this pull request Oct 30, 2024
This updates winit to 0.30.5. 

emilk#4849 Had to pin the version to
0.30.2, as a Winit patch changed the behavior of selecting a theme.
Winit 0.30.5 reverts this, so we could stick with `window.theme()`, but
the newly added `ActiveEventLoop::system_theme` is more like what egui
wants anyway, as individual windows can have theme overrides.

Also bump `smithay-clipboard` to prevent some now duplicate
dependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file eframe Relates to epi and eframe egui-winit porblems related to winit
Projects
None yet
7 participants